Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add docs related to zed/4758 changes #4811

Merged
merged 7 commits into from
Oct 25, 2023
Merged

Add docs related to zed/4758 changes #4811

merged 7 commits into from
Oct 25, 2023

Conversation

philrz
Copy link
Contributor

@philrz philrz commented Oct 20, 2023

This is my attempt to update the docs to reflect what changed in #4758. This took longer than I thought because the flexibility of having -lake, multiple possible environment variables, and the new fallback default paths play with the different zed personalities in many ways. Thankfully the hints from the zed CLI seem to point the user in the right direction if they attempt a combo that's not intended to work, so I stopped short of trying to enumerate every combo and instead continue to lean on what was already written in the "personalities" section to fill in some of the detail (e.g., when URLs can be used vs. file system paths).

I'm expecting 99% of users will stay on the happy path, i.e., when they use zed in its client personality they won't have specified -lake or any environment variables but if their Zui is running it'll have started a lake service and it'll "just work". We do still need to make changes on the Zui side to make it "just work" in the other direction (i.e., so Zui will start defaulting to using storage at %LOCALAPPDATA%\zed on Windows or $HOME/.local/share/zed on Linux/macOS), but that'll be my next conquest once folks sign off on what's in this PR.

All that said, improvements welcomed!

@philrz philrz self-assigned this Oct 20, 2023
cmd/zed/use/command.go Outdated Show resolved Hide resolved
docs/commands/zed.md Outdated Show resolved Hide resolved
docs/commands/zed.md Outdated Show resolved Hide resolved
docs/commands/zed.md Outdated Show resolved Hide resolved
docs/commands/zed.md Outdated Show resolved Hide resolved
philrz and others added 5 commits October 23, 2023 12:06
@philrz philrz requested a review from nwt October 23, 2023 19:27
Comment on lines +179 to +180
3. A Zed lake service running locally at `http://localhost:9867` (if a socket
is listening at that port)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe tighten this?

Suggested change
3. A Zed lake service running locally at `http://localhost:9867` (if a socket
is listening at that port)
3. A Zed lake service running at `http://localhost:9867`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nwt: I actually kept this one un-tight intentionally. During black box testing I noticed that if I run a simple program that just listens on 9867 and accepts the incoming connection, and the lake location logic makes it to this step, zed effectively hangs indefinitely. I guess it's a hair-split to think that the text as I wrote it communicates this subtlety. But I was trying to express that all it takes is a listening socket for it to commit to this step.

#!/usr/local/bin/python3
import socket
import sys

if (len(sys.argv) != 2 or not sys.argv[1].isdigit()):
  print('Usage: listen <port>')
  exit()

p = int(sys.argv[1])
l = []
s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
s.bind(('', p))
s.listen(1)
while 1:
  (c, a) = s.accept()
  l.append(c)
  print('%d: connection from %s' % (len(l), a))

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@philrz: That's a bug!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nwt: Cool! Glad we discussed. I've just opened #4824 to track this.

Comment on lines +181 to +183
4. A `zed` subdirectory below a path in the
[`XDG_DATA_HOME`](https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html)
environment variable (if defined)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe tighten this?

Suggested change
4. A `zed` subdirectory below a path in the
[`XDG_DATA_HOME`](https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html)
environment variable (if defined)
4. `$XDG_DATA_HOME/zed` (if the `XDG_DATA_HOME` environment variable is defined)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nwt: This one was also intentionally un-tight because even on Windows the XDG_DATA_HOME environment variable may be obeyed. Obviously users have their choice of shell on Windows, and some use $ to expand. But since the out-of-the-box Command Prompt uses dual % to expand environment variables, I was keeping it higher level. Maybe another futile hair-split, though.

docs/commands/zed.md Outdated Show resolved Hide resolved
Co-authored-by: Noah Treuhaft <noah.treuhaft@gmail.com>
@philrz philrz merged commit 92b0acd into main Oct 25, 2023
4 checks passed
@philrz philrz deleted the zed-4758-docs branch October 25, 2023 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants